Skip to content

Add individual program length checks into updateApplication#2699

Merged
algojohnlee merged 1 commit into
algorand:masterfrom
algorandskiy:pavel/extra-page-upgrade-2
Aug 6, 2021
Merged

Add individual program length checks into updateApplication#2699
algojohnlee merged 1 commit into
algorand:masterfrom
algorandskiy:pavel/extra-page-upgrade-2

Conversation

@algorandskiy
Copy link
Copy Markdown
Contributor

@algorandskiy algorandskiy commented Aug 6, 2021

Summary

updateApplication does not enforce individual program length but looks like it should. This PR enforces that.

Test Plan

Added tests

@jannotti
Copy link
Copy Markdown
Contributor

jannotti commented Aug 6, 2021 •
edited
Loading

Isn't this enough, in WellFormed?

		lap := len(tx.ApprovalProgram)
		lcs := len(tx.ClearStateProgram)
		pages := int(1 + effectiveEPP)
		if lap > pages*proto.MaxAppProgramLen {
			return fmt.Errorf("approval program too long. max len %d bytes", pages*proto.MaxAppProgramLen)
		}
		if lcs > pages*proto.MaxAppProgramLen {
			return fmt.Errorf("clear state program too long. max len %d bytes", pages*proto.MaxAppProgramLen)
		}

I suppose not, since effectiveEPP is "conservative" in the sense that it can be set higher than a tight bound.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 6, 2021 •
edited
Loading

Codecov Report

Merging #2699 (78e28e9) into master (f3671c0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2699   +/-   ##
=======================================
  Coverage   47.09%   47.09%           
=======================================
  Files         349      349           
  Lines       55882    55887    +5     
=======================================
+ Hits        26315    26319    +4     
- Misses      26619    26620    +1     
  Partials     2948     2948
Impacted Files Coverage Δ
ledger/apply/application.go 81.06% <100.00%> (+0.47%) ⬆️
agreement/cryptoVerifier.go 75.73% <0.00%> (-2.21%) ⬇️
crypto/merkletrie/trie.go 66.42% <0.00%> (-2.19%) ⬇️
agreement/proposalManager.go 96.07% <0.00%> (-1.97%) ⬇️
crypto/merkletrie/node.go 91.62% <0.00%> (-1.87%) ⬇️
ledger/acctupdates.go 61.88% <0.00%> (-0.17%) ⬇️
data/transactions/verify/txn.go 48.45% <0.00%> (ø)
catchup/service.go 69.35% <0.00%> (+0.77%) ⬆️
ledger/blockqueue.go 82.18% <0.00%> (+1.14%) ⬆️
network/wsPeer.go 77.15% <0.00%> (+2.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3671c0...78e28e9. Read the comment docs.

@algojohnlee algojohnlee merged commit 7e84d77 into algorand:master Aug 6, 2021
bricerisingalgorand pushed a commit that referenced this pull request Aug 6, 2021
@algorandskiy @bricerisingalgorand
Add individual program length checks into updateApplication (#2699)
2e8690d 
updateApplication does not enforce individual program length but looks like it should. This PR enforces that.
@onetechnical onetechnical mentioned this pull request Aug 13, 2021
Merged
This was referenced Aug 19, 2021
Closed
Merged
@bricerisingalgorand bricerisingalgorand mentioned this pull request Aug 27, 2021
Merged
@algorandskiy algorandskiy deleted the pavel/extra-page-upgrade-2 branch March 16, 2026 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jannotti jannotti jannotti approved these changes

@tsachiherman tsachiherman Awaiting requested review from tsachiherman

+1 more reviewer

@shiqizng shiqizng shiqizng approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Team Scytale unplanned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@algorandskiy @jannotti @codecov-commenter @shiqizng @algojohnlee @chaihoang